Skip to content

Conversation

samiuelson
Copy link
Contributor

@samiuelson samiuelson commented Aug 22, 2025

POS Incremental Sync Timestamp Storage

Description

Summary

Implemented a repository system for storing and retrieving site-specific incremental sync timestamps for POS products and variations using DataStore preferences.

Changes
  • WooPosSyncTimestampRepository: Low-level repository for string timestamp storage with site-specific keys
  • WooPosSyncTimestampManager: High-level manager providing Date object API with GMT timezone handling
Key Features
  • Site-specific storage: Timestamps are isolated per site using dynamic key generation
  • Type-safe API: Date objects for convenience, strings for raw access
  • Error handling: Graceful null site handling with logging

Steps to reproduce

Testing information

This code is not used at the moment. Check if CI is green and if the code looks good.

The tests that have been performed

Above

Images/gif

N/A

  • I have considered if this change warrants release notes and have added them to RELEASE-NOTES.txt if necessary. Use the "[Internal]" label for non-user-facing changes.

@samiuelson samiuelson requested a review from Copilot August 22, 2025 16:42
Copilot

This comment was marked as outdated.

@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Aug 22, 2025

📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.
App Name WooCommerce-Wear Android
Platform⌚️ Wear OS
FlavorJalapeno
Build TypeDebug
Commita2d9caf
Direct Downloadwoocommerce-wear-prototype-build-pr14511-a2d9caf.apk

@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Aug 22, 2025

📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.

App Name WooCommerce Android
Platform📱 Mobile
FlavorJalapeno
Build TypeDebug
Commita2d9caf
Direct Downloadwoocommerce-prototype-build-pr14511-a2d9caf.apk

@samiuelson samiuelson requested a review from Copilot August 22, 2025 19:14
@samiuelson samiuelson added this to the 23.2 milestone Aug 22, 2025
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR implements a repository system for storing and retrieving site-specific incremental sync timestamps for POS products and variations using DataStore preferences.

  • Adds a low-level repository (WooPosSyncTimestampRepository) for string timestamp storage with site-specific keys
  • Implements a high-level manager (WooPosSyncTimestampManager) providing Date object API with GMT timezone handling
  • Includes comprehensive test coverage for the timestamp manager functionality

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
WooPosSyncTimestampRepository.kt Low-level repository for storing/retrieving site-specific timestamps using DataStore
WooPosSyncTimestampManager.kt High-level manager providing Date object API with GMT formatting and parsing
WooPosSyncTimestampManagerTest.kt Comprehensive unit tests for the timestamp manager functionality

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@samiuelson samiuelson marked this pull request as ready for review August 22, 2025 19:18
@samiuelson samiuelson requested a review from malinajirka August 22, 2025 19:18
@codecov-commenter
Copy link

codecov-commenter commented Aug 22, 2025

Codecov Report

❌ Patch coverage is 30.98592% with 49 lines in your changes missing coverage. Please review.
✅ Project coverage is 37.89%. Comparing base (7f6538d) to head (a2d9caf).
⚠️ Report is 76 commits behind head on trunk.

Files with missing lines Patch % Lines
...os/util/datastore/WooPosSyncTimestampRepository.kt 0.00% 49 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##              trunk   #14511      +/-   ##
============================================
- Coverage     37.89%   37.89%   -0.01%     
- Complexity     9362     9374      +12     
============================================
  Files          2024     2026       +2     
  Lines        113785   113856      +71     
  Branches      15092    15107      +15     
============================================
+ Hits          43121    43146      +25     
- Misses        66698    66745      +47     
+ Partials       3966     3965       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@malinajirka malinajirka self-assigned this Aug 25, 2025

suspend fun storeProductsLastSyncTimestamp(timestamp: Date) {
val timestampGmt: String = gmtDateFormat.format(timestamp)
timestampRepository.storeProductsLastSyncTimestamp(timestampGmt)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 I think storing formatted dates has some drawbacks, the main one is that we can't safely change the format without clearing the storage. I'd suggest storing just timestamps - milliseconds since 1970.

Considering Samuel is AFK, @kidinov could you double check my train of thoughts and share whether you agree with this suggestion before I make the change? Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree. Also timestamp just feels cleaner as we don't need to format anything, And btw, gmtDateFormat is not thread safe

Not related to this, but I just realized that even if the date is retrieved from the backend, it’s not something we control. Therefore, there’s no guarantee that the date will be accurate and can change in some cases

@malinajirka
Copy link
Contributor

@kidinov I have updated the API to store timestamps (Long) instead of dates. I also replaced the thread-unsafe DateFormatter with the new java.time API. Since I made quite a few changes and Sam is AFK, would you mind taking over of the review of this PR?

@kidinov kidinov self-assigned this Aug 26, 2025
Copy link
Contributor

@kidinov kidinov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! I left a couple of questions

@malinajirka malinajirka merged commit e50d4c3 into trunk Aug 28, 2025
17 checks passed
@malinajirka malinajirka deleted the pos-local-catalog-sync-timestamp-repo branch August 28, 2025 07:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants